Skip to content

feat(1password-plugin): add useCliWithServiceAccount for memory-constrained headless environments#692

Merged
theoephraim merged 9 commits intomainfrom
copilot/fix-executereadbatch-env-token-issue
May 6, 2026
Merged

feat(1password-plugin): add useCliWithServiceAccount for memory-constrained headless environments#692
theoephraim merged 9 commits intomainfrom
copilot/fix-executereadbatch-env-token-issue

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented May 5, 2026 •
edited
Loading

  • Changeset switched from minor-isolated to minor
  • Remove global boolean flag; pass service account token explicitly
  • Split CLI batching and auth mutex by auth mode:
    • allowAppAuth (desktop app): module-level shared state (same local op login context)
    • useCliWithServiceAccount: per-instance state (each token is an independent auth context)
  • cli-helper.ts remains stateless utilities only
  • Docs website (1password.mdx): added "CLI-based service account auth" subsection and useCliWithServiceAccount param to @initOp() reference
  • Fix TS errors:
    • packages/utils/src/defer.ts: typed the inner new Promise<T>(...) and updated resolve signature to (value: T | PromiseLike<T>) => void so deferred.promise is correctly typed as Promise<T>
    • packages/plugins/1password/src/plugin.ts line 620: changed createDeferredPromise() → createDeferredPromise<string>() to fix inferred return type of readItem

Copilot AI linked an issue May 5, 2026 that may be closed by this pull request
1Password plugin: executeReadBatch strips OP_SERVICE_ACCOUNT_TOKEN from subprocess env, breaking headless CLI auth #691
Closed
@theoephraim
feat(1password-plugin): add useCliWithServiceAccount param for headle…
b2bdf2a 
…ss CLI auth

Agent-Logs-Url: https://github.com/dmno-dev/varlock/sessions/4fe1ccab-22cb-4d56-b5e5-b53bd2c93606

Co-authored-by: theoephraim <1158956+theoephraim@users.noreply.github.com>
Copilot AI changed the title [WIP] Fix executeReadBatch to preserve OP_SERVICE_ACCOUNT_TOKEN in env feat(1password-plugin): add useCliWithServiceAccount for memory-constrained headless environments May 5, 2026
Copilot AI requested a review from theoephraim May 5, 2026 16:23
@theoephraim
Copy link
Copy Markdown
Member

theoephraim commented May 5, 2026

@copilot - switch to minor and get rid of the -isolated instructions so we dont use that anymore. Also this setting should be scoped to an individual instance of the plugin, yet in the the helper it is using a global var.

@theoephraim
refactor(1password-plugin): pass service account token as parameter i…
f0d6d47 
…nstead of global flag

Agent-Logs-Url: https://github.com/dmno-dev/varlock/sessions/a9f1a7e8-25c9-4f94-a10b-1dd124697e25

Co-authored-by: theoephraim <1158956+theoephraim@users.noreply.github.com>
Copy link
Copy Markdown
Contributor Author

Copilot AI commented May 5, 2026

@copilot - switch to minor and get rid of the -isolated instructions so we dont use that anymore. Also this setting should be scoped to an individual instance of the plugin, yet in the the helper ...

Done in f0d6d47:

  • Changeset switched from minor-isolated to minor
  • Removed the global boolean flag (passServiceAccountTokenToCli / enableCliServiceAccountAuth). The token is now passed explicitly as a serviceAccountToken parameter through opCliRead → stored at module level as cliServiceAccountToken (mirroring lockCliToOpAccount) → forwarded into executeReadBatch and execOpCliCommand. In plugin.ts, this.token is passed directly to opCliRead/opCliEnvironmentRead when useCliWithServiceAccount=true, so there are no global side effects.

@theoephraim
Copy link
Copy Markdown
Member

theoephraim commented May 5, 2026

@copilot - while unlikely, in theory, the user could have different settings for different instances. Can we adjust so there are no global side effects, and batching only combines per instance?

Copilot AI and others added 2 commits May 5, 2026 17:46
@theoephraim
refactor(1password-plugin): move CLI batching into OpPluginInstance f…
e7cd572 
…or full per-instance isolation

Agent-Logs-Url: https://github.com/dmno-dev/varlock/sessions/9b68f206-e3be-4973-b9fb-5a5a5c5ddb90

Co-authored-by: theoephraim <1158956+theoephraim@users.noreply.github.com>
@theoephraim
refactor(1password-plugin): move CLI batching into OpPluginInstance f…
2d0c40d 
…or full per-instance isolation

Agent-Logs-Url: https://github.com/dmno-dev/varlock/sessions/9b68f206-e3be-4973-b9fb-5a5a5c5ddb90

Co-authored-by: theoephraim <1158956+theoephraim@users.noreply.github.com>
Copy link
Copy Markdown
Contributor Author

Copilot AI commented May 5, 2026

@copilot - while unlikely, in theory, the user could have different settings for different instances. Can we adjust so there are no global side effects, and batching only combines per instance?

Done in 2d0c40d. All CLI batching state and logic has been moved into OpPluginInstance:

  • cliAuthDeferred (auth mutex), cliBatch (batch queue) are now private instance fields
  • checkCliAuth(), executeCliBatch(), and cliRead() are now private instance methods
  • cli-helper.ts is now purely stateless utilities — execOpCliCommand, opCliEnvironmentRead, processOpCliError, pickProxyEnv, getIdsFromShareLink

Each instance has its own independent batch queue, account, token, and auth mutex — no global side effects at all.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 5, 2026 •
edited
Loading

bumpy-frog

The changes in this PR will be included in the next version bump.

minor Minor releases

  • @varlock/1password-plugin 1.0.2 → 1.1.0

Bump files in this PR

Click here if you want to add another bump file to this PR

This comment is maintained by bumpy.

@theoephraim
Copy link
Copy Markdown
Member

theoephraim commented May 5, 2026

@copilot - the issue is that if using the cli (without a service account token) then you can only be logged in as one account, so the auth mutex should be global. However if you are using service accounts with the CLI, then things must be kept separate.

@theoephraim
refactor(1password-plugin): use shared auth mutex for app-auth, per-i…
d9ad1b3 
…nstance for service account CLI path

Agent-Logs-Url: https://github.com/dmno-dev/varlock/sessions/49bf1429-b718-4d95-aef4-53016650f893

Co-authored-by: theoephraim <1158956+theoephraim@users.noreply.github.com>
Copy link
Copy Markdown
Contributor Author

Copilot AI commented May 5, 2026

@copilot - the issue is that if using the cli (without a service account token) then you can only be logged in as one account, so the auth mutex should be global. However if you are using service acco...

Done in d9ad1b3. The auth mutex and batch are now split by auth mode:

  • allowAppAuth (desktop app): module-level shared appAuthBatch, appAuthDeferred, and lockCliToOpAccount — all app-auth instances share the same local op login context, so the shared mutex prevents duplicate auth prompts across instances.
  • useCliWithServiceAccount: per-instance cliBatch and cliAuthDeferred — each service account token is an independent auth context, so batches and mutexes stay fully isolated per instance.

@theoephraim
Copy link
Copy Markdown
Member

theoephraim commented May 5, 2026

@copilot - add this info to the docs website too

Copy link
Copy Markdown
Contributor Author

Copilot AI commented May 5, 2026

@copilot - add this info to the docs website too

Done in ba9aae0. Added a "CLI-based service account auth (memory-constrained environments)" subsection to the docs website page, covering the use case, requirements, and a diff code example. Also added useCliWithServiceAccount to the @initOp() reference parameter list.

@cloudflare-workers-and-pages
Copy link
Copy Markdown

cloudflare-workers-and-pages Bot commented May 5, 2026 •
edited
Loading

Deploying with  Cloudflare Workers  Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

Status Name Latest Commit Preview URL Updated (UTC)
✅ Deployment successful!
View logs		 varlock-website ba9aae0 Commit Preview URL

Branch Preview URL		 May 05 2026, 08:58 PM

Copilot AI requested a review from theoephraim May 5, 2026 20:57
@theoephraim
Copy link
Copy Markdown
Member

theoephraim commented May 5, 2026

@copilot fix TS errors

Copy link
Copy Markdown
Contributor Author

Copilot AI commented May 5, 2026

@copilot fix TS errors

Fixed in 8d84e40. Two changes:

  • packages/utils/src/defer.ts: typed new Promise<T>(...) and updated resolve to (value: T | PromiseLike<T>) => void so deferred.promise is correctly Promise<T> instead of Promise<unknown>
  • packages/plugins/1password/src/plugin.ts line 620: createDeferredPromise() → createDeferredPromise<string>() to fix the inferred return type of readItem

Build confirms all 24 tasks pass with no TS errors.

@pkg-pr-new
Copy link
Copy Markdown

pkg-pr-new Bot commented May 6, 2026 •
edited
Loading

Open in StackBlitz

npm i https://pkg.pr.new/@varlock/1password-plugin@692

commit: b8461b9

@theoephraim
fix(1password-plugin): redact tokens in debug logs and add comprehens…
b8461b9 
…ive tests

- Redact token and connectToken in setAuth debug output (was logging plaintext)
- Improve debug labels to distinguish auth methods (SDK vs service account CLI vs app auth)
- Add 28 tests covering all resolution paths: service account CLI, app auth CLI, SDK, environments, data types, schema errors, and batch error retry logic
@theoephraim theoephraim marked this pull request as ready for review May 6, 2026 05:19
@theoephraim theoephraim merged commit b7dc771 into main May 6, 2026
26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@theoephraim theoephraim Awaiting requested review from theoephraim

Assignees

@theoephraim theoephraim

Copilot code review Copilot

Labels

core:utils core:website maintainer plugin:1password

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1Password plugin: executeReadBatch strips OP_SERVICE_ACCOUNT_TOKEN from subprocess env, breaking headless CLI auth

2 participants

@theoephraim